feat: Implement vrf router with auto-allocating evpn_vni attribute#2109
feat: Implement vrf router with auto-allocating evpn_vni attribute#2109stevekeay wants to merge 2 commits into
Conversation
|
|
||
|
|
||
| def register_understack_vni_opts(config): | ||
| with contextlib.suppress(cfg.DuplicateOptError): |
There was a problem hiding this comment.
Why was this triggering? This one is a bit concerning to me that we'll get bad data.
There was a problem hiding this comment.
Yes, this should not have been present in the production code - I believe LLM added it to allow it to register different options for various test cases. I have removed it in cbbc38a.
This was added because a test case was registering multiple conflicting options. I am removing because it is no longer required.
| "allow_post": True, | ||
| "allow_put": False, | ||
| "convert_to": converters.convert_to_int_if_not_none, | ||
| "default": 0, |
There was a problem hiding this comment.
should that be 0 or ATTR_NOT_SPECIFIED ? I am concerned if having it as 0 will not prevent non-admin users from creating routers
There was a problem hiding this comment.
I am a bit hazy on the logic for collections, but I think we want non-admin users to create routers, we just don't want them to be able to choose a VNI.
A value of zero is explicitly interpreted as "please auto-assign" in the assignment hook, so the value that actually gets persisted should be non-zero.
Technically it may be more correct to use the sentinel value but I thought this was going to the DB layer, I'm not sure what the best answer is.
There was a problem hiding this comment.
The python/neutron-understack/neutron_understack/conf/policies/evpn.py has:
policy.DocumentedRuleDefault(
name="create_router:evpn_vni",
check_str=base.ADMIN,
scope_types=["project"],
description="Specify ``evpn_vni`` attribute when creating a router",
operations=ACTION_POST,
...
which will translate to "create_router:evpn_vni": "rule:admin_only" inside a default policy (similarly to stock one and if the user provides any value of the attribute in the request, it will match the rule, effectively requiring admin privileges, so we may need to adjust base.ADMIN to sth else.
| neutron-understack = "neutron_understack.conf.policies:list_rules" | ||
|
|
||
| [project.entry-points."neutron.db.alembic_migrations"] | ||
| neutron-understack = "neutron_understack.db.migration:alembic_migrations" |
There was a problem hiding this comment.
that seems to be missing from __init__.py. So it probably needs to have from neutron_understack.db.migration import alembic_migrations or maybe this will work(untested)
| neutron-understack = "neutron_understack.db.migration:alembic_migrations" | |
| neutron-understack = "neutron_understack.db.migration.alembic_migrations" |
| never_used_vni = self._find_never_used_vni(context, ranges) | ||
| if never_used_vni is not None: | ||
| allocation = vni_models.UnderstackRouterVNIAllocation( | ||
| vni=never_used_vni, | ||
| router_id=router_id, | ||
| ) | ||
| context.session.add(allocation) | ||
| context.session.flush() | ||
| return never_used_vni | ||
|
|
||
| released = self._find_released_allocation(context, ranges) | ||
| if released: | ||
| released.router_id = router_id | ||
| released.released_at = None | ||
| context.session.flush() | ||
| return released.vni |
There was a problem hiding this comment.
While slim chances with our workload, there is a risk of concurrent auto allocations picking the same never-used VNI. The faster one gets it, the loser will get 500 error. Consider catching a duplicate violation and retrying for more graceful handling:
| never_used_vni = self._find_never_used_vni(context, ranges) | |
| if never_used_vni is not None: | |
| allocation = vni_models.UnderstackRouterVNIAllocation( | |
| vni=never_used_vni, | |
| router_id=router_id, | |
| ) | |
| context.session.add(allocation) | |
| context.session.flush() | |
| return never_used_vni | |
| released = self._find_released_allocation(context, ranges) | |
| if released: | |
| released.router_id = router_id | |
| released.released_at = None | |
| context.session.flush() | |
| return released.vni | |
| for _attempt in range(3): | |
| never_used_vni = self._find_never_used_vni(context, ranges) | |
| if never_used_vni is not None: | |
| try: | |
| allocation = vni_models.UnderstackRouterVNIAllocation( | |
| vni=never_used_vni, | |
| router_id=router_id, | |
| ) | |
| context.session.add(allocation) | |
| context.session.flush() | |
| return never_used_vni | |
| except db_exc.DBDuplicateEntry: | |
| context.session.rollback() | |
| continue # another worker won; recompute candidate | |
| released = self._find_released_allocation(context, ranges) | |
| if released: | |
| released.router_id = router_id | |
| released.released_at = None | |
| context.session.flush() | |
| return released.vni | |
| raise UnderstackVNINoAvailable(ranges=format_vni_ranges(ranges)) | |
| raise UnderstackVNINoAvailable(ranges=format_vni_ranges(ranges)) |
There was a problem hiding this comment.
There is also oslo_db_api.wrap_db_retry decorator which may work here
@oslo_db_api.wrap_db_retry(
max_retries=3,
retry_on_deadlock=True,
exception_checker=lambda exc: isinstance(exc, db_exc.DBDuplicateEntry),
)|
|
||
| raise UnderstackVNINoAvailable(ranges=format_vni_ranges(ranges)) | ||
|
|
||
| def _allocate_specific_vni(self, context, router_id, vni): |
There was a problem hiding this comment.
similarly to the above, it might be worth wrapping the context.session.flush() with catch for duplicates, although the retry would not make any sense here
try:
context.session.flush()
except db_exc.DBDuplicateEntry:
context.session.rollback()
raise UnderstackVNIInUse(vni=vni, router_id="unknown")or something like that
|
|
||
| MIN_VNI = getattr(n_const, "MIN_VXLAN_VNI", 1) | ||
| MAX_VNI = n_const.MAX_VXLAN_VNI | ||
| DEFAULT_VNI_RANGES = [f"{MIN_VNI}:{MAX_VNI}"] |
3 participants
This creates a standalone plugin for 2026.1 to provide the vrf router behaviour we need, including auto-assignment of VNIs from a configured range.
Some of this overlaps work in progress upstream. I have intentionally mirrored the --evpn-vni API surface of the evpn plugin. We are not using any of the actual evpn plugin code but (in theory) once it gains feature parity, we should be able to switch over to it without affecting our users.
This got fairly big. I was thinking it might be cleaner to break it out into a separate package (that we would install separately via pip install in the container) but I wanted to solicit opinions from the team.
To use this:
service_pluginsremovingevpnand addingunderstack_vni[understack_vni]section likevni_ranges = 10000:19999